fix: deflake run_kubectl_apply_tests #93657

knight42 · 2020-08-03T16:53:29Z

Signed-off-by: knight42 anonymousknight96@gmail.com

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:

/kind api-change
/kind bug
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature

/kind flake

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #93651

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

liggitt · 2020-08-03T17:04:45Z

test/cmd/apply.sh

@@ -444,6 +444,9 @@ run_kubectl_server_side_apply_tests() {
 }
 __EOF__

+  # Ensure the CRD has been created


This is not the correct check... the CRD will always be created here

The issue is whether the API server has recognized and started serving the associated CR API yet

@liggitt Is it possible to do a wait on a Condition for a CRD? (e.g. kubectl wait --for=condition=Ready mygroup.example.com/myobj --timeout=30s)

I think we want to wait until kubectl observes the expected API available in discovery (kubectl api-resources contains the new API)

@seans3 you could wait for condition Established, although I am not certain that guarantees that it is being served

kubernetes/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/types.go

Line 279 in 2585d78

Established CustomResourceDefinitionConditionType = "Established"

knight42 · 2020-08-04T03:08:44Z

/test pull-kubernetes-conformance-kind-ipv6-parallel

hasheddan · 2020-08-04T16:11:56Z

test/cmd/apply.sh

+          break
+      fi
+      echo "${i}: Waiting for CR API to be available"
+      sleep $((i-1))


I may be missing something but why the changing sleep time? Is it intended to be a rudimentary backoff?

no special reason, just following the existing practice

kubernetes/hack/lib/test.sh

Lines 82 to 93 in c381615

for j in $(seq 1 "${tries}"); do

# shellcheck disable=SC2086

# Disabling because "args" needs to allow for expansion here

res=$(eval kubectl get "${kube_flags[@]}" ${args} "${object}" -o go-template=\""${request}"\")

if [[ "${res}" =~ ^$expected$ ]]; then

echo -n "${green}"

echo "$(kube::test::get_caller 3): Successful get ${object} ${request}: ${res}"

echo -n "${reset}"

return 0

fi

echo "Waiting for Get ${object} ${request} ${args}: expected: ${expected}, got: ${res}"

sleep $((j-1))

we could fix the sleep time if that is preferred

@knight42 I think it's okay, just wanted to know the motivation :)

this is similar, used in other places, and is easier to read:

Suggested change

sleep $((i-1))

sleep "${i}"

hasheddan · 2020-08-05T13:35:47Z

Saw another flake on this last night. @liggitt could you give this another look?

liggitt · 2020-08-05T13:37:44Z

test/cmd/apply.sh

+  # Ensure the API server has recognized and started serving the associated CR API
+  local tries=5
+  for i in $(seq 1 $tries); do
+      if kubectl "${kube_flags_with_token[@]}" api-resources --api-group 'mygroup.example.com' -oname | grep -q 'resources.mygroup.example.com'; then


piping to grep -q breaks things, see #57805

liggitt · 2020-08-05T14:16:29Z

test/cmd/apply.sh

+  # Ensure the API server has recognized and started serving the associated CR API
+  local tries=5
+  for i in $(seq 1 $tries); do
+      local output=$(kubectl "${kube_flags_with_token[@]}" api-resources --api-group 'mygroup.example.com' -oname)


Suggested change

local output=$(kubectl "${kube_flags_with_token[@]}" api-resources --api-group 'mygroup.example.com' -oname)

local output=$(kubectl "${kube_flags[@]}" api-resources --api-group mygroup.example.com -o name)

liggitt · 2020-08-05T14:40:56Z

/lgtm
/approve

liggitt · 2020-08-05T14:41:17Z

/milestone v1.19

test-only change addressing a flake clouding CI signal

k8s-ci-robot · 2020-08-05T14:41:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: knight42, liggitt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~test/cmd/OWNERS~~ [liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: knight42 <anonymousknight96@gmail.com>

knight42 · 2020-08-05T15:28:26Z

Force push to address shellcheck errors: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/93657/pull-kubernetes-verify/1291016793187422209

Errors from shellcheck:

In ./test/cmd/apply.sh line 450:
      local output=$(kubectl "${kube_flags[@]:?}" api-resources --api-group mygroup.example.com -oname)
            ^----^ SC2155: Declare and assign separately to avoid masking return values.


In ./test/cmd/apply.sh line 455:
      sleep $i
            ^-- SC2086: Double quote to prevent globbing and word splitting.

Did you mean: 
      sleep "$i"

liggitt · 2020-08-05T16:23:05Z

/lgtm

k8s-ci-robot requested review from derekwaynecarr and mengqiy August 3, 2020 16:54

k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 3, 2020

knight42 mentioned this pull request Aug 3, 2020

[Flaky Test] run_kubectl_apply_tests (ci-kubernetes-integration-master) #93651

Closed

liggitt reviewed Aug 3, 2020

View reviewed changes

knight42 force-pushed the test/93651 branch from 2921555 to e243b57 Compare August 3, 2020 17:35

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 3, 2020

knight42 requested a review from liggitt August 3, 2020 17:36

hasheddan reviewed Aug 4, 2020

View reviewed changes

liggitt reviewed Aug 5, 2020

View reviewed changes

knight42 force-pushed the test/93651 branch from e243b57 to 41d180a Compare August 5, 2020 14:05

knight42 requested a review from liggitt August 5, 2020 14:05

liggitt reviewed Aug 5, 2020

View reviewed changes

knight42 force-pushed the test/93651 branch from 41d180a to 8a30de8 Compare August 5, 2020 14:22

knight42 requested a review from liggitt August 5, 2020 14:24

k8s-ci-robot assigned liggitt Aug 5, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 5, 2020

k8s-ci-robot added this to the v1.19 milestone Aug 5, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 5, 2020

fix: deflake run_kubectl_apply_tests

cbf8ec4

Signed-off-by: knight42 <anonymousknight96@gmail.com>

knight42 force-pushed the test/93651 branch from 8a30de8 to cbf8ec4 Compare August 5, 2020 15:27

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 5, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 5, 2020

k8s-ci-robot merged commit c94242a into kubernetes:master Aug 5, 2020

knight42 deleted the test/93651 branch August 5, 2020 17:21

knight42 mentioned this pull request Aug 14, 2020

test(kubectl): deflake run_kubectl_apply_tests(round 2) #93990

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: deflake run_kubectl_apply_tests #93657

fix: deflake run_kubectl_apply_tests #93657

knight42 commented Aug 3, 2020

liggitt Aug 3, 2020

seans3 Aug 3, 2020

liggitt Aug 3, 2020

hasheddan Aug 3, 2020

knight42 commented Aug 4, 2020

hasheddan Aug 4, 2020

knight42 Aug 4, 2020 •

edited

knight42 Aug 4, 2020

hasheddan Aug 4, 2020

liggitt Aug 5, 2020

hasheddan commented Aug 5, 2020

liggitt Aug 5, 2020

liggitt Aug 5, 2020

liggitt commented Aug 5, 2020

liggitt commented Aug 5, 2020

k8s-ci-robot commented Aug 5, 2020

knight42 commented Aug 5, 2020 •

edited

liggitt commented Aug 5, 2020

	for j in $(seq 1 "${tries}"); do
	# shellcheck disable=SC2086
	# Disabling because "args" needs to allow for expansion here
	res=$(eval kubectl get "${kube_flags[@]}" ${args} "${object}" -o go-template=\""${request}"\")
	if [[ "${res}" =~ ^$expected$ ]]; then
	echo -n "${green}"
	echo "$(kube::test::get_caller 3): Successful get ${object} ${request}: ${res}"
	echo -n "${reset}"
	return 0
	fi
	echo "Waiting for Get ${object} ${request} ${args}: expected: ${expected}, got: ${res}"
	sleep $((j-1))

	local output=$(kubectl "${kube_flags_with_token[@]}" api-resources --api-group 'mygroup.example.com' -oname)
	local output=$(kubectl "${kube_flags[@]}" api-resources --api-group mygroup.example.com -o name)

fix: deflake run_kubectl_apply_tests #93657

fix: deflake run_kubectl_apply_tests #93657

Conversation

knight42 commented Aug 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knight42 commented Aug 4, 2020

Choose a reason for hiding this comment

knight42 Aug 4, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hasheddan commented Aug 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Aug 5, 2020

liggitt commented Aug 5, 2020

k8s-ci-robot commented Aug 5, 2020

knight42 commented Aug 5, 2020 • edited

liggitt commented Aug 5, 2020

knight42 Aug 4, 2020 •

edited

knight42 commented Aug 5, 2020 •

edited